-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Spec edits for @defer/@stream #742
Conversation
spec/Section 5 -- Validation.md
Outdated
@@ -411,6 +411,7 @@ FieldsInSetCanMerge(set): | |||
{set} including visiting fragments and inline fragments. | |||
* Given each pair of members {fieldA} and {fieldB} in {fieldsForName}: | |||
* {SameResponseShape(fieldA, fieldB)} must be true. | |||
* {SameStreamDirective(fieldA, fieldB)} must be true. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SameStreamAndDeferDirective
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our proposal limits @defer
to fragments and fragment spreads, so only @stream
provides a new opportunity for overlapping fields to conflict.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@robrichard did you mind elaborating why this is the case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have concerns about this validation rule; namely I think it breaks "composability" of a GraphQL query. If you have a fragment somewhere that contains { friends { name } }
and another fragment that contains { friends @stream { name } }
then these two fragments are composible under current GraphQL rules but would not be composable once this rule is introduced.
I feel like these fragments could remain composable if we don't add this rule. Since "stream" is optional, in the above case GraphQL could determine that it has to evaluate the entire list up front anyway so it could just return the same set for both.
Similarly if you had { friends @stream(initialCount: 1) { name } }
and { friends @stream(initialCount: 5) { name } }
these could both be merged into a single { friends @stream(initialCount: 5) { name } }
since (as @AndrewIngram points out in the WG chat) the initialCount: 5
would satisfy both.
(This consideration makes the no-stream effectively analogous to initialCount: ∞
.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That said, I like the simplicity it introduces. So I'm definitely on the fence for this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We did originally do something like that, but received guidance from facebook recommending a validation rule
See discussion here: graphql/graphql-js#2319 (comment)
Following up on: graphql/graphql-js#2319 (comment) I am still not quite sure on use case for label with stream if different labels are not allowed. probably just missing something, but if it could be elaborated upon with an example of a conflict, it would be most appreciated. Thanks! |
@yaacovCR There's a possibility for conflict when you combine defer and stream. I posted an example here: https://gist.github.com/robrichard/0a5e9fb7b0e615545198183c2b9c95a8 |
Well, defer needs a label in that example, but does stream? |
Thanks so much for your quick reply!!! |
My unsolicited two cents from the feedback so far based on the different ways fragments are processed and the implementing code is that the directive should be named patch instead of defer, as in theory, this is not a regular fragment of fields at all, but an indication to split off a new branch of execution from the existing operation, and would be better off with a new keyword in fact but for the need to preserve backwards compatibility. |
@yaacovCR in this proposal, label is an optional argument to both For |
Wow, you have clarified everything! Thanks so much for your time. I do still disagree as to the name of the defer directive. It seems the first conceptual point is that a new branch of execution is being created via a patch with a response that is possibly deferred. It is up to the server in fact to indicate whether it is indeed delayed at all. Perhaps defer means deferred to another payload, rather than another timepoint, but that is somewhat unclear. It seems like a patch is a more appropriate, more general term, as defer more aptly describes how the payload is to be delivered, which might be better off in an argument to the proposed patch directive, with more granular options even than defer. Other than that, again, this conversation has been tremendously clarifying for me, and very helpful to me at least in organizing some initial thoughts in terms of integrating schema stitching with these changes. I very much appreciate your time and all the work that has gone into this spec change by @lilianammmatos, yourself and all others involved, apologies to all I am missing. |
Although I suppose patch is not quite right for defer either as a patch might be a stream payload... Maybe directive |
This comment has been minimized.
This comment has been minimized.
c8e289c
to
87ec5dc
Compare
This comment has been minimized.
This comment has been minimized.
87ec5dc
to
097dbff
Compare
spec/Section 6 -- Execution.md
Outdated
ResolveStreamRecord(streamRecord, variableValues, subsequentPayloads): | ||
* Let {label}, {iterator}, {resolvedItems}, {index}, {path}, {fields}, | ||
{innerType} be the correspondent fields on the Stream Record structure. | ||
* Remove the first entry from {resolvedItem}, let the entry be {item}. If | ||
{resolvedItem} is empty, retrieve more items from {iterator}: | ||
* Append {index} to {path}. | ||
* Increment {index}. | ||
* Let {payload} be the result of calling CompleteValue(innerType, fields, item, variableValues, subsequentPayloads, path)}. | ||
* Add an entry to {payload} named `label` with the value {label}. | ||
* Add an entry to {payload} named `path` with the value {path}. | ||
* If {resolveItem} is not empty or {iterator} does not reach the end: | ||
* Append {streamRecord} to {subsequentPayloads}. | ||
* Return {payload}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you might need to revisit this definition; I might be reading it wrong but it feels like you may have used resolvedItem
in the place of resolvedItems
in one place, and the "retrieve more items from {iterator}" isn't super clear, it refers to "If {resolvedItem} is not empty" under an item which already seems to state "If {resolvedItem} is empty", and there doesn't seem to be a point at which {resolvedItem}
would change between the two. Also it uses resolveItem
rather than resolvedItem
which I think is a typo since it doesn't seem to be referred to elsewhere.
Sorry if this has been discussed elsewhere; but what if you don't know if the current payload is going to be the final payload? Is sending a payload with just Let me back this question up with a concrete example: price comparison websites. Imagine we visit a price comparison website, PCW, and search for home insurance. PCW's back-end goes off to it's panel of 500 home insurance providers and asks them each for a quote with your details. Some of the providers offer a quote, others decline, and they take varying amounts of time to do so up to a maximum cut-off time of 2 minutes. Lets say 90 providers give a quote. In GraphQL without In an ideal In the currently proposed |
@benjie thanks for taking a look. It is our intention for the spec to support a final payload of |
Awesome 🙌 |
Co-authored-by: Simon Gellis <[email protected]>
94363c9
to
c630301
Compare
and some baseline collection language for comparison extracted from graphql#742
and some baseline collection language for comparison extracted from graphql#742 Authored-by: Rob Richard <[email protected]>
and some baseline collection language for comparison extracted from graphql#742 Authored-by: Rob Richard <[email protected]>
and some baseline collection language for comparison extracted from graphql#742 Authored-by: Rob Richard <[email protected]>
and some baseline collection language for comparison extracted from graphql#742 Authored-by: Rob Richard <[email protected]>
and some baseline collection language for comparison extracted from graphql#742 Authored-by: Rob Richard <[email protected]>
When the `path` field is present on a Defer payload, it indicates that the | ||
`data` field represents the result of the fragment containing the corresponding | ||
`@defer` directive. The path segments must point to the location of the result | ||
of the field containing the associated `@defer` directive. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the defer is at the root level then there is no field, in that case the path would be empty I think.
query { ...@defer {person(id: "cGVvcGxlOjE=") {name}}
|
||
The `incremental` entry in the response is a non-empty list of Defer or Stream | ||
payloads. If the response of the GraphQL operation is a response stream, this | ||
field may appear on both the initial and subsequent values. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"this field" - maybe "this entry" since "field" has specific meaning in GraphQL.
and some baseline collection language for comparison extracted from graphql#742 Authored-by: Rob Richard <[email protected]>
Continued in #1110 |
Corresponding
graphql-js
pull request: graphql/graphql-js#2319Please keep discussion limited to specifics on how the spec text is written. There is a dedicated discussion forum for questions, feedback, and comments on defer & stream: https://github.com/robrichard/defer-stream-wg/discussions